-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement StoreSelectedScalar for Arm64 #93223
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsContribute towards #84510. // AdvSimd
public static unsafe void StoreSelectedScalar64x2(byte* address, (Vector64<byte> Value1, Vector64<byte> Value2) value, byte index);
public static unsafe void StoreSelectedScalar64x2(sbyte* address, (Vector64<sbyte> Value1, Vector64<sbyte> Value2) value, byte index);
public static unsafe void StoreSelectedScalar64x2(short* address, (Vector64<short> Value1, Vector64<short> Value2) value, byte index);
public static unsafe void StoreSelectedScalar64x2(ushort* address, (Vector64<ushort> Value1, Vector64<ushort> Value2) value, byte index);
public static unsafe void StoreSelectedScalar64x2(int* address, (Vector64<int> Value1, Vector64<int> Value2) value, byte index);
public static unsafe void StoreSelectedScalar64x2(uint* address, (Vector64<uint> Value1, Vector64<uint> Value2) value, byte index);
public static unsafe void StoreSelectedScalar64x2(float* address, (Vector64<float> Value1, Vector64<float> Value2) value, byte index);
// AdvSimd.Arm64
public static unsafe void StoreSelectedScalar128x2(byte* address, (Vector128<byte> Value1, Vector128<byte> Value2) value, byte index);
public static unsafe void StoreSelectedScalar128x2(sbyte* address, (Vector128<sbyte> Value1, Vector128<sbyte> Value2) value, byte index);
public static unsafe void StoreSelectedScalar128x2(short* address, (Vector128<short> Value1, Vector128<short> Value2) value, byte index);
public static unsafe void StoreSelectedScalar128x2(ushort* address, (Vector128<ushort> Value1, Vector128<ushort> Value2) value, byte index);
public static unsafe void StoreSelectedScalar128x2(int* address, (Vector128<int> Value1, Vector128<int> Value2) value, byte index);
public static unsafe void StoreSelectedScalar128x2(uint* address, (Vector128<uint> Value1, Vector128<uint> Value2) value, byte index);
public static unsafe void StoreSelectedScalar128x2(long* address, (Vector128<long> Value1, Vector128<long> Value2) value, byte index);
public static unsafe void StoreSelectedScalar128x2(ulong* address, (Vector128<ulong> Value1, Vector128<ulong> Value2) value, byte index);
public static unsafe void StoreSelectedScalar128x2(float* address, (Vector128<float> Value1, Vector128<float> Value2) value, byte index);
public static unsafe void StoreSelectedScalar128x2(double* address, (Vector128<double> Value1, Vector128<double> Value2) value, byte index);
|
4672ea0
to
621dc68
Compare
This patch would emit the following assembly sequence 1. When the index is a constant valueAdvSimd.StoreSelectedScalar64x2((Int32*)_dataTable.outArrayPtr, (AdvSimd.LoadVector64((Int32*)(_dataTable.inArray1Ptr)), AdvSimd.LoadVector64((Int32*)(_dataTable.inArray2Ptr))), 1);
2. When index is not a constantAdvSimd.StoreSelectedScalar64x2((Int32*)_dataTable.outArrayPtr, (AdvSimd.LoadVector64((Int32*)(_dataTable.inArray1Ptr)), AdvSimd.LoadVector64((Int32*)(_dataTable.inArray2Ptr))), elemIndex);
3. When the index is variable and out of boundsThe current test crashes with an error.
Not sure whether it's the desired behaviour. Also, I haven't considered the possibility of creating a ROP gadget with the sequence for Case 2 above. The sequence of instruction where value of an index can be used control the jump target. |
As seen in #93197, the API names should be just |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will double check about the immediate operand out of range scenario.
Alright, so if you follow #93197, the |
They should throw |
src/tests/JIT/HardwareIntrinsics/Arm/Shared/StoreSelScalarx2Test.template
Outdated
Show resolved
Hide resolved
Interesting. What is the cause of it? |
Now we get out of range exception for all the invalid scenarios (with constant or variable index) with all the test cases.
This mystery is also resolved now. There was a check for array out of bounds missing. Now added it. Can be confirmed from the assembly for
Full assembly
Interestingly, for some tests I got |
Seems the indices of few test cases needs adjustment for them to not throw Exception. |
Had to move the range check after recomputing the |
Sure! It's tricky to debug, and it isn't obvious why compilation of |
From what I remember, these tests were passing earlier and failing with the recent commit and are in arm64, so most likly related. I could be wrong. |
You're right, it seems to be related to the arg check. Not sure how to debug it though. I'll try to see if I can reproduce the failure. If that doesn't progress, should I put the explicit check as before? |
I would say just inspect the code changes and see if the code path for existing |
@SwapnilGaikwad - can you resolve the merge conflicts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change to Mono codebase looks good to me.
Change-Id: I81d386adbbcc195b777f436329f24438b0cca10c
@fanyang-mono - was the change essentially to make sure that mono continue to ignore newer APIs whose 2nd parameter is tuple and only support the existing |
Unfortunately, yes, Mono is not super friendly with handling API with 4 and more input arguments. We need to add additional code to handle those cases, rather than automatically handled by the machinery like Vector64/128. There is an issue to track Mono side of the work related to these new set of API's. |
Contribute towards #84510.